Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tensor wave 1 API's. #101196

Merged
merged 25 commits into from
May 16, 2024
Merged

Tensor wave 1 API's. #101196

merged 25 commits into from
May 16, 2024

Conversation

michaelgsharp
Copy link
Member

This is the initial PR for the Tensor Prototype. The design doc is here, and its still in discussion so updates will be coming from the result of those discussions. Further updates will also be coming in regards to error handling/etc, but this will allow for preliminary reviews.

@michaelgsharp michaelgsharp self-assigned this Apr 17, 2024
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

@build-analysis build-analysis bot mentioned this pull request Apr 17, 2024
Open
@michaelgsharp michaelgsharp force-pushed the tensor-prototype branch 2 times, most recently from a4ce322 to 61faee1 Compare April 22, 2024 21:17
@michaelgsharp michaelgsharp marked this pull request as ready for review April 22, 2024 21:17
stephentoub
stephentoub reviewed Apr 22, 2024
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've skimmed through some. Will review the rest in a bit.

src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs Show resolved Hide resolved
This was referenced Apr 23, 2024
Open
Open
Closed
Closed
Closed
stephentoub
stephentoub reviewed Apr 23, 2024
View reviewed changes
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs Outdated
{
public static partial class Tensor
{
public static Tensor<T> Create<T>(bool mustPin, ReadOnlySpan<nint> lengths)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing XML comments on a bunch of these public APIs

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs Outdated Show resolved Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs Outdated Show resolved Hide resolved
@michaelgsharp michaelgsharp changed the title Tensor prototype Tensor wave 1 API's. Apr 23, 2024
This was referenced Apr 26, 2024
Open
Closed
Closed
Open
tannergooding
tannergooding reviewed May 15, 2024
View reviewed changes
src/libraries/System.Numerics.Tensors/ref/System.Numerics.Tensors.netcore.cs
public bool MoveNext() { throw null; }
}
}
public static partial class Tensor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these APIs that both take and return some form of Span are missing scoped where relevant.

Many of the span APIs are also unnecessarily mutable, for example Add should only be taking in ReadOnlyTensorSpan while AddInPlace should take one TensorSpan and one ReadOnlyTensorSpan.

I'm fine with this class in particular being handled in a follow up PR, but I think it's important to do that cleanup prior to the first preview.

tannergooding
tannergooding reviewed May 15, 2024
View reviewed changes
src/libraries/System.Numerics.Tensors/ref/System.Numerics.Tensors.netcore.cs Outdated
static System.Numerics.Tensors.Tensor<T> System.Numerics.Tensors.ITensor<System.Numerics.Tensors.Tensor<T>, T>.CreateUninitialized(scoped System.ReadOnlySpan<nint> lengths, scoped System.ReadOnlySpan<nint> strides, bool pinned) { throw null; }
public string ToString(scoped System.ReadOnlySpan<nint> maximumLengths) { throw null; }
public bool TryCopyTo(scoped System.Numerics.Tensors.TensorSpan<T> destination) { throw null; }
public bool TryFlattenTo(scoped System.Span<T> destination) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise fine with it being cleaned up in a separate PR, but scoped shouldn't be needed on any API where the underlying ref field cannot be captured.

Tensor<T> cannot have a ref field and most of these APIs do not return a ref. It's different for TensorSpan<T> since the hidden this parameter allows the ref to be captured.

tannergooding
tannergooding approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a lot of cleanup that still needs to be done in the form of optimization work, validating inputs are correct/sensible to the BCL/FDG standards, ensuring we do the relevant security audit, etc

But, this looks like it now matches the so far reviewed API proposal and should be good to merge to keep the PR from growing exponentially. We should ideally log issues to track most of this additional work/cleanup prior to merging

@tannergooding
Copy link
Member

tannergooding commented May 15, 2024 •
edited
Loading

This was referenced May 15, 2024
Open
Open
Closed
@michaelgsharp michaelgsharp merged commit b15e351 into dotnet:main May 16, 2024
83 checks passed
@matouskozak matouskozak mentioned this pull request May 17, 2024
Open
@michaelgsharp michaelgsharp deleted the tensor-prototype branch May 22, 2024 19:02
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@michaelgsharp @Ruihan-Yin
Tensor wave 1 API's. (dotnet#101196)
258a010 
* Native Index/Range and ref.

* SpanND without slice

* index/range testing and implicit conversions

* SpanND Tests

* tensor working, still need more statics

* more tensors updates

* ref files updated

* final ref update

* span updates

* all but broadcast and some TensorPrimitives

* broadcast in

* organizational changes

* ref and implicit broadcast

* build failures

* updates from PR comments

* error text moved to strings.resx

* exception strings moved to strings.resc

* comments from PR

* more fixes from PR and API review

* rebase on main. XML comments. API updates

* NIndex,NRange,RO/TensorSpan API updates

* IROTensor,ITensor,Tensor API updates

* changes from pr comments

* changes from PR comments

* fixed test failure
@github-actions github-actions bot locked and limited conversation to collaborators Jun 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@tannergooding tannergooding tannergooding approved these changes

@jeffhandley jeffhandley Awaiting requested review from jeffhandley

@NiklasGustafsson NiklasGustafsson Awaiting requested review from NiklasGustafsson

@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis

@ericstj ericstj Awaiting requested review from ericstj

@JakeRadMSFT JakeRadMSFT Awaiting requested review from JakeRadMSFT

@luisquintanilla luisquintanilla Awaiting requested review from luisquintanilla

Assignees

@michaelgsharp michaelgsharp

Labels
area-System.Numerics new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@michaelgsharp @tannergooding @stephentoub